Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Packet.MarshalTo() thread-safe #168

Merged
merged 1 commit into from
Mar 17, 2022
Merged

Conversation

aler9
Copy link
Member

@aler9 aler9 commented Mar 8, 2022

Description

A Marshal function shouldn't change any property of the object that is being marshaled. Otherwise, Marshal() is non-thread safe
and can't be called by multiple goroutines in parallel.

PR #155 makes Packet.Marshal() set the Packet.Header.Padding bit when Packet.PaddingSize is non-zero; while this is preferable from the user point of view, it breaks thread safety.

This patch fixes the issue.

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #168 (0a08f0e) into master (beba640) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   86.32%   86.31%   -0.01%     
==========================================
  Files          15       15              
  Lines        1638     1637       -1     
==========================================
- Hits         1414     1413       -1     
  Misses        195      195              
  Partials       29       29              
Flag Coverage Δ
go 86.31% <ø> (-0.01%) ⬇️
wasm 85.82% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packet.go 87.94% <ø> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beba640...0a08f0e. Read the comment docs.

@aler9 aler9 force-pushed the patch-padding branch 2 times, most recently from 0743dc5 to 2c3722f Compare March 8, 2022 15:38
@aler9 aler9 changed the title make Packet.MarshalTo() constant and restore thread safety make Packet.MarshalTo() thread-safe Mar 8, 2022
@aler9 aler9 changed the title make Packet.MarshalTo() thread-safe Make Packet.MarshalTo() thread-safe Mar 8, 2022
@aler9
Copy link
Member Author

aler9 commented Mar 8, 2022

This issue was discovered through gortsplib's unit tests - in the tests, there's a dummy rtp.Packet that is shared between all tests, and is marshaled by multiple routines at the same time.

@Sean-Der
Copy link
Member

Thanks for fixing this @aler9! I don't fully understand the impact, but this doesn't seem to cause any test fails so should be ok? I would think this branch wouldn't be used anymore and tests would fail.

@boks1971 mind reviewing/any opinion.

@boks1971
Copy link
Contributor

Thank you @aler9 . This would mean call sites would have set the proper flags which is fine.

I am not totally happy about the handling of padding myself. But, I have not found a better way. Just to give a bit of history about the changes I made

  • SFUs peek into the payload to determine things like spatial/temporal layer and if a packet is a key frame or not. They were not checking if the packet is padding only (padding only packets used by clients to probe for bandwidth). Some clients did not zero out the padding bytes. So, the parsing ended up doing incorrect things. The way to address that would have been to check for Padding flag in Header and then look at the last byte of payload and check it against length to determine the actual payload size. Making apps do that was a bit cumbersome. So, my first commit was to chop the payload and get rid of payload bytes. Apps could just check if len(Payload) == 0 to check for padding only.
  • Then I realised that the information about how big the packet actually was after unmarshaling. So, I added the PaddingSize field. And to ensure that Marshal does the right thing if some body just provides PaddingSize, I did the change which caused this issue.

I think it is fine that call sites have to provide the proper Packet to Marshal. Thank you again!

@aler9
Copy link
Member Author

aler9 commented Mar 17, 2022

@boks1971 i would have done it the same way. I also thought about dropping the padding, but i think that this choice must be left to the user. This library should just be able to unmarshal any compliant RTP packet and marshal it as it is. I like bijective functions: every output corresponds to only one input and vice-versa.

As for my user case, i use this library for handling RTP packets generated by cameras, and they add a lot of padding that can be filtered without problems.

A Marshal function shouldn't change any property of the object that is
being marshaled. Otherwise, Marshal() is non-thread safe and can't be
called by multiple goroutines in parallel.

PR pion#155 makes Packet.Marshal() set the Packet.Header.Padding bit when
Packet.PaddingSize is non-zero; while this is preferable from the user
point of view, it doesn't allow thread safety.

This patch fixes the issue.
@aler9 aler9 merged commit 7772c8f into pion:master Mar 17, 2022
@aler9 aler9 deleted the patch-padding branch March 17, 2022 11:26
aler9 added a commit to aler9/rtp that referenced this pull request Jul 28, 2023
This was already fixed by pion#168 but got lost in pion#227. in SFUs, in order
to distribute a packet to all clients, MarshalTo() is called in
parallel by multiple routines, causing a race condition because the
padding flag is dynamically set inside MarshalTo(). This is particular
annoying when running automated tests. This PR fixes the issue by
removing this write operation as discussed in pion#168.
aler9 added a commit that referenced this pull request Jul 28, 2023
This was already fixed by #168 but got lost in #227. in SFUs, in order
to distribute a packet to all clients, MarshalTo() is called in
parallel by multiple routines, causing a race condition because the
padding flag is dynamically set inside MarshalTo(). This is particular
annoying when running automated tests. This PR fixes the issue by
removing this write operation as discussed in #168.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants